Having both FunctionABIInfo and ABIFunctionInfo is pretty confusing... do these need to be separate?

um, I've renamed it for the time being - if I feel it doesn't serve a purpose in a few days I'll put it somewhere else

Hmm... this is tricky. I'm not entirely sure what to do here. For the purpose of mapping ABIArgInfo, we should never encounter unions. For the struct case, things are tricky. Mostly, I believe we just want LLVM structs as a means of holding multiple values. E.g. a StructType for Direct arguments will normally get unpacked into individual arguments. In this case, we really don't want any kind of padding fillers in it. I think the only case that cares about that is CoerceAndExpand, which has two struct types, one just with the elements (which will be passed as separate arguments) and one that has the actual layout (with correct alignment etc).

This kind of makes me wonder whether it would make sense to start by storing llvm types in ABIArgInfo to match what clang currently does. abi::Type is conceptually nicer, but also not quite the right representation for everything (e.g. because it can't really represent the "struct without layout" case well).

we should never encounter unions

Oh yeah! My bad...

Also I'm assuming both times you referred to ABIArgInfo you were talking about abi::ABIArgInfo and not codegen::ABIArgInfo (should probably name that something slightly different sorry).

Given that assumption - I really don't think we need to bring llvm::Type into the library as the struct without layout case can be added to the ABI typesystem itself (add another type of abi::StructPacking or decide whether we need padding while converting). But agreed a lot of cleanup to be done here, I'll keep it in mind while writing the x86 target.

Also I'm assuming both times you referred to ABIArgInfo you were talking about abi::ABIArgInfo and not codegen::ABIArgInfo (should probably name that something slightly different sorry).

Yeah, I mean abi::ABIArgInfo. Btw, I wonder whether we should call these abi::ArgInfo etc, given that we already have abi in the namespace name...

Given that assumption - I really don't think we need to bring llvm::Type into the library as the struct without layout case can be added to the ABI typesystem itself (add another type of abi::StructPacking or decide whether we need padding while converting). But agreed a lot of cleanup to be done here, I'll keep it in mind while writing the x86 target.

Yeah, this is an option. I was mostly thinking about using llvm::Type as the path of least resistance to clang integration...

um, I've renamed it for the time being - if I feel it doesn't serve a purpose in a few days I'll put it somewhere else

we should never encounter unions

Oh yeah! My bad...

Also I'm assuming both times you referred to ABIArgInfo you were talking about abi::ABIArgInfo and not codegen::ABIArgInfo (should probably name that something slightly different sorry).

Given that assumption - I really don't think we need to bring llvm::Type into the library as the struct without layout case can be added to the ABI typesystem itself (add another type of abi::StructPacking or decide whether we need padding while converting). But agreed a lot of cleanup to be done here, I'll keep it in mind while writing the x86 target.

A note for the future: We probably need to do something to avoid leaking memory from these temporary types. We can probably checkpoint the type arena before computing ABIArgInfo and then restore the checkpoint once it's no longer needed.

You can use something like LLVM_PREFERRED_TYPE(bool) unsigned IsTransparent : 1; etc to pack these into bits.

As far as I can tell, this HasUnalignedFields property is unused.

Where does this condition come from? Comparing to

Doesn't it need to account for VTable Pointers, and other C++ ABI affecting factors like dtors. Like:

struct test {
    virtual void foo();
    int x;
};

Was just a hunch, I could remove it if its trivial and get rid of those flags from the typesystem in that case...

That does make sense, but what I don't really get is why these things aren't checked in the existing Clang code. I think maybe the assumption is that this has already been handled previously by getRecordArgABI(), so records like that won't reach to this point? (In which case I guess the main needed property is CanPassInRegisters?)

Hmm maybe... I'll give it a try